Skip to content

Unify primitive and decimal physical storage#8864

Draft
connortsui20 wants to merge 2 commits into
developfrom
ct/fixed-width-primitive-decimal
Draft

Unify primitive and decimal physical storage#8864
connortsui20 wants to merge 2 commits into
developfrom
ct/fixed-width-primitive-decimal

Conversation

@connortsui20

@connortsui20 connortsui20 commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • introduce shared FixedWidthData / FixedWidthType storage for canonical Primitive and Decimal arrays
  • retain distinct Primitive and Decimal VTables, logical behavior, matchers, and existing array IDs
  • preserve the existing Primitive and Decimal serialized metadata and buffer/validity layouts
  • update Decimal call sites to use its distinct canonical representation

Compatibility

The existing vortex.primitive and vortex.decimal IDs and their serialized layouts remain unchanged. The historical compatibility suite passed all 455 published fixtures across Vortex 0.64.0 and 0.68.0 through 0.79.0 on the combined implementation.

@connortsui20 connortsui20 mentioned this pull request Jul 20, 2026
Draft
@github-actions

github-actions Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Polar Signals Profiling Results

Latest Run

Status Commit Job Attempt Link
🟢 Done 80db289 1 Explore Profiling Data
Previous Runs (7)
Status Commit Job Attempt Link
🟢 Done 8f8869b 1 Explore Profiling Data
🟢 Done f124538 1 Explore Profiling Data
🟢 Done 2880b5f 1 Explore Profiling Data
🟢 Done 35a4157 1 Explore Profiling Data
🟢 Done 2e0b1e2 1 Explore Profiling Data
🟢 Done f00395e 1 Explore Profiling Data
🟢 Done 3ba3236 1 Explore Profiling Data

Powered by Polar Signals Cloud

@codecov

codecov Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.25228% with 176 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.03%. Comparing base (2ea51cb) to head (f00395e).

Files with missing lines Patch % Lines
vortex-array/src/arrays/decimal/array.rs 64.56% 45 Missing ⚠️
vortex-array/src/arrays/decimal/compute/cast.rs 80.58% 20 Missing ⚠️
vortex-array/src/arrays/fixed_width.rs 81.18% 19 Missing ⚠️
vortex-array/src/arrays/decimal/vtable/mod.rs 62.79% 16 Missing ⚠️
vortex-cuda/src/arrow/canonical.rs 0.00% 16 Missing ⚠️
vortex-cuda/src/kernel/arrays/dict.rs 0.00% 10 Missing ⚠️
vortex-array/src/arrays/primitive/vtable/mod.rs 73.91% 6 Missing ⚠️
vortex-array/src/arrays/decimal/compute/mask.rs 58.33% 5 Missing ⚠️
vortex-array/src/arrays/decimal/compute/rules.rs 61.53% 5 Missing ⚠️
vortex-array/src/arrays/decimal/compute/take.rs 83.33% 4 Missing ⚠️
... and 15 more

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmarks: Vortex queries 📖

Verdict: No clear signal (low confidence)
Attributed Vortex impact: +4.6%
Engines: DataFusion No clear signal (+6.3%, low confidence) · DuckDB No clear signal (+2.9%, low confidence)
Vortex (geomean): 1.044x ➖
Parquet (geomean): 1.021x ➖
Shifts: Parquet (control) +2.1% · Median polish +4.1%

How to read Verdict and Engines
  • Verdict: Overall PR-level signal after subtracting baseline drift estimated from Parquet control rows. It can be Likely improvement, Likely regression, or No clear signal.
  • Engines: Per-engine attribution. DataFusion is compared against DataFusion/Parquet controls; DuckDB is compared against DuckDB/Parquet controls. This answers whether each engine improved or regressed independently.
  • Confidence: Based on directional consistency, share of rows above the noise floor, and control-run noise.
datafusion / vortex-file-compressed (1.079x ➖, 0↑ 1↓)
name PR 80db289 (ns) base b7ee873 (ns) ratio (PR/base)
vortex_q00/datafusion:vortex-file-compressed 10023422 9472638 1.06
vortex_q01/datafusion:vortex-file-compressed 🚨 6542586 5947344 1.10
datafusion / parquet (1.015x ➖, 0↑ 0↓)
name PR 80db289 (ns) base b7ee873 (ns) ratio (PR/base)
vortex_q00/datafusion:parquet 20620031 20526611 1.00
vortex_q01/datafusion:parquet 4716001 4597130 1.03
duckdb / vortex-file-compressed (1.056x ➖, 0↑ 0↓)
name PR 80db289 (ns) base b7ee873 (ns) ratio (PR/base)
vortex_q00/duckdb:vortex-file-compressed 10425542 10002024 1.04
vortex_q01/duckdb:vortex-file-compressed 6241644 5833329 1.07
duckdb / parquet (1.026x ➖, 0↑ 0↓)
name PR 80db289 (ns) base b7ee873 (ns) ratio (PR/base)
vortex_q00/duckdb:parquet 23623816 23225002 1.02
vortex_q01/duckdb:parquet 9650203 9318548 1.04

No file size changes detected.

@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.11%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 1875 untouched benchmarks
⏩ 3 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation copy_nullable[65536] 1 ms 1.4 ms -24.17%
❌ Simulation copy_non_nullable[65536] 913.5 µs 1,094.1 µs -16.51%
⚡ Simulation new_raw_prim_test_between[i32, 2048] 38.8 µs 33.8 µs +14.72%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

Comparing ct/fixed-width-primitive-decimal (80db289) with develop (b7ee873)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 2 times, most recently from 76fd71a to f00395e Compare July 20, 2026 19:50
connortsui20
connortsui20 commented Jul 20, 2026
View reviewed changes
Comment thread vortex-array/src/array/erased.rs Outdated
robert3005
robert3005 reviewed Jul 20, 2026
View reviewed changes
Comment thread vortex-array/src/array/erased.rs Outdated
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 2 times, most recently from 35a4157 to 2e0b1e2 Compare July 20, 2026 22:05
@connortsui20 connortsui20 mentioned this pull request Jul 20, 2026
Merged
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch 2 times, most recently from 35a4157 to 2880b5f Compare July 20, 2026 22:33
@connortsui20 connortsui20 mentioned this pull request Jul 20, 2026
Open
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch from 2880b5f to f124538 Compare July 20, 2026 22:42
connortsui20 added a commit that referenced this pull request Jul 21, 2026
@connortsui20
Remove unused ArrayRef::is_arrow (#8868)
39f4abe 
## Summary

- remove the unused `ArrayRef::is_arrow` helper
- keep this reviewer-requested cleanup separate from #8864

## Testing

- `cargo check -p vortex-array`
- `cargo clippy --all-targets --all-features` (pre-push hook)

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch from f124538 to 8f8869b Compare July 21, 2026 17:26
@connortsui20
Move primitive storage behind fixed-width abstraction
2f727c2 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
Reuse fixed-width storage for decimal arrays
80db289 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the ct/fixed-width-primitive-decimal branch from 8f8869b to 80db289 Compare July 21, 2026 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robert3005 robert3005 robert3005 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

changelog/feature A new feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@connortsui20 @robert3005